Support passing preimage for spontaneous payments #549

aagbotemi · 2025-05-31T12:00:12Z

This PR adds support for specifying custom preimages when sending spontaneous (keysend) payments.

Solution

Added send_with_preimage() method
Added send_with_preimage_and_custom_tlvs() method for advanced use cases
Modified internal send_inner() to accept optional preimage parameter and generate random preimage only when custom preimage is not provided.

An integration test is added to verify logic, and the new methods are added to the bindings.

Fixes #535

ldk-reviews-bot · 2025-05-31T12:00:15Z

👋 I see @TheBlueMatt was un-assigned.
If you'd like another reviewer assignemnt, please click here.

chuksys

Just shared a few thoughts regarding the overall design of this API and its extensibility.

chuksys · 2025-06-01T08:43:14Z

src/payment/spontaneous.rs

+	/// Send a spontaneous with custom preimage
+	pub fn send_with_preimage(
+		&self, amount_msat: u64, node_id: PublicKey, sending_parameters: Option<SendingParameters>,
+		preimage: PaymentPreimage,


I'm wondering if we should expand the purpose of SendingParameters a bit to allow passing of any custom parameters required for sending payments (not just routing and pathfinding parameters).

This approach would allow us to include preimage as an Option within SendingParameters, providing a single, coherent place for all payment-sending customizations.

A significant benefit of this change would be the ability to deprecate or drop specialized functions like send_with_preimage and send_with_custom_tlvs (in another PR), reducing overall boilerplate for users.

What do you think about this?

I initially hesitated about mixing routing parameters with payment content. Your point about expanding SendingParameters to handle all payment customization makes sense for the long-term API design, and the benefit outweigh the conceptual mixing.

I'll refactor to move preimage into SendingParameters as an Option. I can drop the send_with_preimage and send_with_preimage_and_custom_tlvs, then send_with_custom_tlvs can be dropped in another PR.

Thank you.

I'm wondering if we should expand the purpose of SendingParameters a bit to allow passing of any custom parameters required for sending payments (not just routing and pathfinding parameters).

Ah, no, I don't think we should do that. Note that after LDK 0.2 release (cf. #462) we'll replace SendingParameters with LDK's RouteParametersConfig everywhere, so really shouldn't make any changes prior to it. Also, while we might be fine to expose the preimage for keysend payments, I don't think we'd like to allow users to override it for BOLT11/BOLT12 generally.

I'll refactor to move preimage into SendingParameters as an Option.

Please revert this. I think we can either have separate send APIs as you previously suggested, or add the preimage as an optional field to the existing ones. Will take a look after the revert, but your previous approach might be fine.

Ah, no, I don't think we should do that. Note that after LDK 0.2 release (cf. #462) we'll replace SendingParameters with LDK's RouteParametersConfig everywhere, so really shouldn't make any changes prior to it. Also, while we might be fine to expose the preimage for keysend payments, I don't think we'd like to allow users to override it for BOLT11/BOLT12 generally.

Thanks for the clarification!

chuksys · 2025-06-01T15:39:47Z

src/payment/spontaneous.rs

+		self.send_inner(amount_msat, node_id, sending_parameters, Some(custom_tlvs), None)
+	}
+
+	/// Send a spontaneous with custom preimage


I think you missed the word "payment" here (after spontaneous) - that's if we'd still be adding this method.

Yeah, I missed that. Thank you. I am removing the method.

chuksys

If we are going to use sending_parameters to pass in the custom preimage, I think we should allow the user to set it in config or pass it directly.

chuksys · 2025-06-02T07:09:13Z

src/payment/spontaneous.rs

@@ -77,7 +77,10 @@ impl SpontaneousPayment {
 			return Err(Error::NotRunning);
 		}

-		let payment_preimage = PaymentPreimage(self.keys_manager.get_secure_random_bytes());
+		let payment_preimage = sending_parameters


If we are doing this, I think we'll need to use override_params instead as that allows send_inner to select sending_parameters set in the config (if not passed into the send method directly).

No, I don't think we should do this, see above.

tnull

As mentioned above I don't think adding this field to SendingParameters is the way to go here (note it would also allowing setting global default preimage if users would override the field in Config, which is actually dangerous as preimages can never be reused). Please revert to you previous draft, will review then.

tnull · 2025-06-02T11:15:53Z

src/payment/spontaneous.rs

@@ -77,7 +77,10 @@ impl SpontaneousPayment {
 			return Err(Error::NotRunning);
 		}

-		let payment_preimage = PaymentPreimage(self.keys_manager.get_secure_random_bytes());
+		let payment_preimage = sending_parameters


No, I don't think we should do this, see above.

aagbotemi · 2025-06-02T12:21:07Z

Please revert to you previous draft, will review then.

Alright. Will do that. Thank you.

aagbotemi · 2025-06-02T13:30:41Z

Please revert to you previous draft, will review then.

Hello @tnull, I have reverted to the previous draft. This is ready.

Camillarhi · 2025-06-02T18:43:48Z

Nice work on this PR! The custom preimage functionality is implemented cleanly, and the test coverage looks good.

One small enhancement suggestion: we could add verification that node_b received the payment to complete end-to-end verification of the payment flow.

aagbotemi · 2025-06-03T09:09:53Z

One small enhancement suggestion: we could add verification that node_b received the payment to complete end-to-end verification of the payment flow.

Thanks for the suggestion! I've added the end-to-end verification as requested. It also ensure the custom preimage flows correctly through the entire payment process from sender to receiver.

tnull

Basically LGTM, but now that we actually take a PaymentPreimage from user input we should probably avoid the UniffiCustomTypeConverter for it, and rather expose a 'proper' newtype wrapper for the bindings. See #542 for inspiration on this, for example.

bindings/ldk_node.udl

tnull · 2025-06-03T10:21:10Z

src/payment/spontaneous.rs

+	/// Send a spontaneous payment with custom preimage
+	pub fn send_with_preimage(
+		&self, amount_msat: u64, node_id: PublicKey, sending_parameters: Option<SendingParameters>,
+		preimage: PaymentPreimage,


If we add this as a new required parameter, let's move it before the optional parameters, i.e., Option<SendingParameters> (here and below).

Alright, will fix it. Thank you.

aagbotemi · 2025-06-04T09:02:13Z

Basically LGTM, but now that we actually take a PaymentPreimage from user input we should probably avoid the UniffiCustomTypeConverter for it, and rather expose a 'proper' newtype wrapper for the bindings. See #542 for inspiration on this, for example.

Thank you for the feedback, I will fix it

chuksys

Generally looks good to me - would appreciate some clarity on why we have maybe_wrap and maybe_wrap_arc.

chuksys · 2025-06-25T22:38:06Z

src/ffi/mod.rs

+pub fn maybe_wrap_arc<T>(ldk_type: impl Into<T>) -> std::sync::Arc<T> {
 	std::sync::Arc::new(ldk_type.into())
 }

+#[cfg(feature = "uniffi")]
+pub fn maybe_wrap<T>(ldk_type: impl Into<T>) -> T {
+	ldk_type.into()
+}
+


I'm probably missing something here but I'm wondering why we need to "split" maybe_wrap into these 2 functions.

maybe_wrap and maybe_wrap_arc is to handle different type wrapping requirements depending on whether the uniffi feature is enabled. maybe_wrap_arc wraps the input type in a std::sync::Arc<T>. maybe_wrap performs a simple into() conversion without additional wrapping, used in cases where the type doesn’t need to be wrapped in an Arc.

From my understanding, I thought maybe_wrap (as it was before) already handled wrapping depending on whether the uniFFI feature flag is enabled - if enabled wrap in an Arc, if not return same value as passed in?

I noticed maybe_wrap (this new version) is used to conditionally wrap the preimage - does this mean the preimage never needs to be wrapped in an Arc even when the uniFFI feature flag is enabled?

In #542, the wrapper types introduced were for structs with a bit more complexity and state than PaymentPreimage. From my understanding, it makes sense to allocate them on the heap because they are also expensive to clone. PaymentPreimage is simpler, wraps 32 bytes, and should be cheaper to clone and doesn't need to be Arc'ed.

Also, when compiled with uniffi, there was a bit of conflict about converting maybe_wrap into a macro so it can handle the different cases (i.e. types needing heap allocation vs types that do not), however I think @tnull may not like the idea for reasons stated in #526.

This change is easily revertible though if Arcing is preferred.

In #542, the wrapper types introduced were for structs with a bit more complexity and state than PaymentPreimage. From my understanding, it makes sense to allocate them on the heap because they are also expensive to clone. PaymentPreimage is simpler, wraps 32 bytes, and should be cheaper to clone and doesn't need to be Arc'ed.

Thanks for the clarification! This makes sense.

chuksys · 2025-06-25T22:45:55Z

src/ffi/mod.rs

+#[cfg(not(feature = "uniffi"))]
+pub fn maybe_wrap_arc<T>(value: T) -> T {
+	value
+}
+
 #[cfg(not(feature = "uniffi"))]
 pub fn maybe_wrap<T>(value: T) -> T {
 	value
 }


These 2 fns are basically the same, please is there a reason why we need both of them?

both maybe_wrap_arc and maybe_wrap are identical, simply returning the input value. The reason for keeping them as separate functions is to maintain a consistent API that aligns with their distinct behaviors when the uniffi feature is enabled

aagbotemi · 2025-06-25T23:24:12Z

Hi @tnull @enigbe. I pushed some update that expose a 'proper' newtype wrapper for the bindings.

enigbe · 2025-06-26T10:05:43Z

src/ffi/mod.rs

+pub fn maybe_wrap_arc<T>(ldk_type: impl Into<T>) -> std::sync::Arc<T> {
 	std::sync::Arc::new(ldk_type.into())
 }

+#[cfg(feature = "uniffi")]
+pub fn maybe_wrap<T>(ldk_type: impl Into<T>) -> T {
+	ldk_type.into()
+}
+


In #542, the wrapper types introduced were for structs with a bit more complexity and state than PaymentPreimage. From my understanding, it makes sense to allocate them on the heap because they are also expensive to clone. PaymentPreimage is simpler, wraps 32 bytes, and should be cheaper to clone and doesn't need to be Arc'ed.

Also, when compiled with uniffi, there was a bit of conflict about converting maybe_wrap into a macro so it can handle the different cases (i.e. types needing heap allocation vs types that do not), however I think @tnull may not like the idea for reasons stated in #526.

This change is easily revertible though if Arcing is preferred.

enigbe · 2025-06-26T10:28:45Z

bindings/ldk_node.udl

@@ -446,6 +450,10 @@ dictionary PaymentDetails {
 	u64 latest_update_timestamp;
 };

+dictionary PaymentPreimage {
+	bytes inner;


I wonder about what would be the most appropriate description for [u8;N]: bytes vs sequence<u8>?

bytes represents fixed-size binary data like preimage better

enigbe · 2025-06-26T10:35:53Z

@aagbotemi
Just added a couple of comments.
You'll need to give CI another kick to get the failing Rust Tests to pass.

tnull

When migrating PaymentPreimage to use its own wrapper type, it's important to just use this type in the Uniffi API. It should just wrap the inner type and not add any additional logic that might get stale over time. In particular, we def. shouldn't add specific serialization logic, as we'd still hold LdkPaymentPreimage internally, and just convert it at the API boundaries if necessary. To this end, please try to avoid adding more conversion wrapper methods if they are not really necessary (and I don't think they are in this case?).

It would be great if you could split this PR in two commits: a first commit introducing the Uniffi wrapper for PaymentPreimage, the second one exposing the new APIs for spontaneous payments.

Lastly, please try to write more conclusive commit messages that describe the changes, but also provide rationale for the changes. Feel free to refer to https://cbea.ms/git-commit/ for some guidance here. Thanks!

tnull · 2025-06-27T07:53:30Z

src/ffi/types.rs

-	type Builtin = String;
+#[derive(Debug, Clone, PartialEq, Eq)]
+pub struct PaymentPreimage {
+	pub(crate) inner: Vec<u8>,


For consistency reasons, please just store an LdkPaymentPreimage as an inner.

Alright. Thank you

tnull · 2025-06-27T07:54:14Z

src/ffi/types.rs

+	}
+}
+
+impl Readable for PaymentPreimage {


No, we really shouldn't roll our own custom serialization PaymentPreimage here, we just use the wrapper type in the API, for nothing else.

Alright. Noted

tnull · 2025-06-27T07:55:54Z

src/ffi/mod.rs

@@ -18,6 +18,14 @@ where
 	wrapped_type.as_ref().as_ref()
 }

+#[cfg(feature = "uniffi")]
+pub fn maybe_extract<T, R>(wrapped_type: T) -> Result<R, crate::error::Error>


I don't think we need this, as we could just reuse maybe_try_convert_enum (which could be renamed to maybe_try_from, maybe).

In general, I'd prefer it if we wouldn't add any more wrapper helpers in this PR, I think (?) we should be able to make it work with what's there, no?

Okay, I will make use of the wrapper helper we currently have.

aagbotemi · 2025-06-28T08:36:46Z

When migrating PaymentPreimage to use its own wrapper type, it's important to just use this type in the Uniffi API. It should just wrap the inner type and not add any additional logic that might get stale over time. In particular, we def. shouldn't add specific serialization logic, as we'd still hold LdkPaymentPreimage internally, and just convert it at the API boundaries if necessary. To this end, please try to avoid adding more conversion wrapper methods if they are not really necessary (and I don't think they are in this case?).

It would be great if you could split this PR in two commits: a first commit introducing the Uniffi wrapper for PaymentPreimage, the second one exposing the new APIs for spontaneous payments.

Lastly, please try to write more conclusive commit messages that describe the changes, but also provide rationale for the changes. Feel free to refer to https://cbea.ms/git-commit/ for some guidance here. Thanks!

Thank you for the feedback @tnull. I will address the comments and push the updated changes.

enigbe · 2025-07-01T09:52:14Z

When migrating PaymentPreimage to use its own wrapper type, it's important to just use this type in the Uniffi API. It should just wrap the inner type and not add any additional logic that might get stale over time. In particular, we def. shouldn't add specific serialization logic, as we'd still hold LdkPaymentPreimage internally, and just convert it at the API boundaries if necessary.

@aagbotemi initially tried this approach, representing PaymentPreimage as an interface/object that wraps LdkPaymentPreimage. However, because the LightningBalance enum has a variant struct with PaymentPreimage field, we are unable to just wrap the inner type as is because there is no support for interfaces/objects in an enum.

thread 'main' panicked at build.rs:10:59:
  called `Result::unwrap()` on an `Err` value: Objects cannot currently be used in enum variant data
  note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

I recommended representing PaymentPreimage as a dictionary to circumvent this error, but that means we have to add specific serialization for this, as has been done.

We are currently using

uniffi = { version = "0.27.3", features = ["build"], optional = true }

and this error originates from this:

# uniffi_udl-0.27.3/src/converters/callables.rs
impl APIConverter<FieldMetadata> for weedle::argument::SingleArgument<'_> {
    fn convert(&self, ci: &mut InterfaceCollector) -> Result<FieldMetadata> {
        let type_ = ci.resolve_type_expression(&self.type_)?;
        if let Type::Object { .. } = type_ {
            bail!("Objects cannot currently be used in enum variant data");
        }
        if self.default.is_some() {
            bail!("enum interface variant fields must not have default values");
        }
        if self.attributes.is_some() {
            bail!("enum interface variant fields must not have attributes");
        }
        // TODO: maybe we should use our own `Field` type here with just name and type,
        // rather than appropriating record::Field..?
        Ok(FieldMetadata {
            name: self.identifier.0.to_string(),
            ty: type_,
            default: None,
            docstring: None,
        })
    }
}

but may have been addressed in "0.29.3".

@tnull any reservations about updating uniffi to explore this? any suggestions on how to handle this without additional logic is welcome.

tnull · 2025-07-01T11:39:32Z

However, because the LightningBalance enum has a variant struct with PaymentPreimage field, we are unable to just wrap the inner type as is because there is no support for interfaces/objects in an enum.

Ah, no, I think we are able to go this way, but it unfortunately mean we'll have to create a bindings-specific variant of LightningBalance that holds the respective field in an Arc and switch it around based on the uniffi-feature, too.

but may have been addressed in "0.29.3".

Good to know!

@tnull any reservations about updating uniffi to explore this? any suggestions on how to handle this without additional logic is welcome.

Yeah, unfortunately we can't upgrade to 0.29.3: for one, it's MSRV is 1.82, while we're on 1.75. But, the more important aspect is that we have users that use LDK Node in conjuction with uniffi-bindgen-go, which is only compatible with 0.28 currently. I.e., if we upgraded and made use of newer feature, they'd need to downgrade and apply the same workarounds in their fork, which I'd like to avoid (cc @rolznz). But we should be able to upgrade to 0.28 by now, which we now do in #591, even though this would likely not make our lives easier in this PR, sadly.

enigbe · 2025-07-02T18:27:21Z

Ah, no, I think we are able to go this way, but it unfortunately mean we'll have to create a bindings-specific variant of LightningBalance that holds the respective field in an Arc and switch it around based on the uniffi-feature, too.

Thanks for this pointer.

tnull

Alright, sorry, this PR fell a bit off my radar and up until recently I wasn't aware that it was still blocked on the best way of exposing a PaymentPreimage object in Uniffi bindings.

Given that it's been open for a while now, I'd suggest to decouple the two tasks after all to make some progress. I.e., let's just revert this PR to have only the changes to 'support passing preimage for spontaneous payments', and then we can see whether/how we can expose the PaymentPreimage better/safer in a follow-up.

FWIW, we might actually punt on it until we can upgrade to uniffi 0.29 given that it seems it would make our lives considerably easier, as @enigbe suggested above. (which would be the case once uniffi-bindgen-go makes a move on their pending upgrade PR. Note that we're about to bump MSRV with #606).

aagbotemi · 2025-08-11T11:06:13Z

I'd suggest to decouple the two tasks after all to make some progress. I.e., let's just revert this PR to have only the changes to 'support passing preimage for spontaneous payments',

Alright, I'll revert the PR. Thank you.

tnull

Generally looks good, but there are still a lot of unrelated/unnecessary changes in the diff. Please revert them. This branch also needs a rebase to allow CI to pass.

tnull · 2025-08-12T12:30:30Z

src/payment/bolt11.rs

@@ -539,6 +539,7 @@ impl Bolt11Payment {
 		} else {
 			None
 		};
+


nit: Spurious newline.

Okay, noted

tnull · 2025-08-12T12:31:29Z

src/payment/store.rs

@@ -221,40 +221,40 @@ impl StorableObject for PaymentDetails {
 				},
 			}
 		}
-		if let Some(preimage_opt) = update.preimage {
+		if let Some(preimage_opt) = &update.preimage {


Not sure why these changes taking-by-reference were introduced, but I don't think we don't need any of them, especially if they'd require us to unnecessarily clone values?

I have fixed it.

aagbotemi · 2025-08-12T22:43:23Z

Generally looks good, but there are still a lot of unrelated/unnecessary changes in the diff. Please revert them. This branch also needs a rebase to allow CI to pass.

I have fixed the unrelated changed, and reverted where necessary. Thank you.

tnull

Thanks, LGTM

ldk-reviews-bot requested a review from TheBlueMatt May 31, 2025 12:10

chuksys reviewed Jun 1, 2025

View reviewed changes

chuksys reviewed Jun 2, 2025

View reviewed changes

tnull removed the request for review from TheBlueMatt June 2, 2025 11:15

tnull reviewed Jun 2, 2025

View reviewed changes

tnull reviewed Jun 3, 2025

View reviewed changes

aagbotemi force-pushed the support-preimage-spontaneous-payment branch 2 times, most recently from 2ffdc53 to 5194ccc Compare June 25, 2025 21:04

chuksys reviewed Jun 25, 2025

View reviewed changes

enigbe reviewed Jun 26, 2025

View reviewed changes

aagbotemi force-pushed the support-preimage-spontaneous-payment branch from 68a141d to c33d291 Compare June 26, 2025 13:53

tnull reviewed Jun 27, 2025

View reviewed changes

tnull reviewed Aug 11, 2025

View reviewed changes

aagbotemi force-pushed the support-preimage-spontaneous-payment branch 3 times, most recently from 3a87ab4 to 3aa16b0 Compare August 12, 2025 11:27

tnull reviewed Aug 12, 2025

View reviewed changes

aagbotemi force-pushed the support-preimage-spontaneous-payment branch from 3aa16b0 to 2807540 Compare August 12, 2025 21:58

feat: add ability to set payment preimage for spontaneous payments

90cde61

aagbotemi force-pushed the support-preimage-spontaneous-payment branch from 2807540 to 90cde61 Compare August 12, 2025 22:13

tnull approved these changes Aug 13, 2025

View reviewed changes

tnull merged commit a147ad0 into lightningdevkit:main Aug 13, 2025
15 checks passed

Support passing preimage for spontaneous payments #549

Support passing preimage for spontaneous payments #549

Uh oh!

Conversation

aagbotemi commented May 31, 2025

Solution

Uh oh!

ldk-reviews-bot commented May 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chuksys left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aagbotemi Jun 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chuksys left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tnull left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aagbotemi commented Jun 2, 2025

Uh oh!

aagbotemi commented Jun 2, 2025

Uh oh!

Camillarhi commented Jun 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aagbotemi commented Jun 3, 2025

Uh oh!

tnull left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aagbotemi commented Jun 4, 2025

Uh oh!

chuksys left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aagbotemi Jun 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aagbotemi commented Jun 25, 2025

Uh oh!

Choose a reason for hiding this comment

ldk-reviews-bot commented May 31, 2025 •

edited

Loading

aagbotemi Jun 1, 2025 •

edited

Loading

Camillarhi commented Jun 2, 2025 •

edited

Loading

aagbotemi Jun 25, 2025 •

edited

Loading

tnull left a comment •

edited

Loading